Skip to content

Expose Replace* methods for 2.x - #2435

Merged
Amir-61 merged 1 commit into
2.xfrom
expose_endpoints_2.x
Aug 15, 2016
Merged

Expose Replace* methods for 2.x#2435
Amir-61 merged 1 commit into
2.xfrom
expose_endpoints_2.x

Conversation

@Amir-61

@Amir-61 Amir-61 commented Jun 14, 2016

Copy link
Copy Markdown
Member

Patch description:

*Re-mapping updateAttributes endpoint to use
PATCH and PUT(configurable) verb
*Exposing replaceById and replaceOrCreate via
POST and PUT(configurable) verb


What are the differences between this ptach for 2.x Vs. patch for 3.x:
  1. Default value for replaceOnPUT would be set to false; please see here

  2. In this patch for 2.x, we preserve upsert and set pachOrCreate as an alias only and change tests accordingly.

  3. In this patch for 2.x, we preserve updateAttributes and set pachAttributes as an alias only and change tests accordingly.

  4. Question: JSdoc for PersistedModel Methods are generated based on lib\persisted-models.js, so once this patch gets landed I believe docs for replaceOrCreate, replaceAttributes and replaceById gets added to our API docs, are we fine with that now?

PS: I squash my commits as one single commit after getting an LGTM. Thanks!

Connect to https://github.com/strongloop-internal/scrum-loopback/issues/917

/to: @bajtos

@Amir-61 Amir-61 changed the title Expose Replace* methods Expose Replace* methods for 2.x Jun 15, 2016
@Amir-61 Amir-61 removed their assignment Jun 15, 2016
@bajtos

bajtos commented Jun 20, 2016

Copy link
Copy Markdown
Member

Question: JSdoc for PersistedModel Methods are generated based on lib\persisted-models.js, so once this patch gets landed I believe docs for replaceOrCreate, replaceAttributes and replaceById gets added to our API docs, are we fine with that now?

I don't see why it would be a bad thing to expose API docs for the two new methods, what am I missing?

@bajtos bajtos assigned Amir-61 and unassigned bajtos Jun 20, 2016
Comment thread lib/persisted-model.js
// This is just for LB 2.x
options.replaceOnPUT = options.replaceOnPUT === true;

function setRemoting(scope, name, options) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be set to false by default, shouldn't it?

@Amir-61 Amir-61 Jun 20, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bajtos:

I believe options.replaceOnPUT = options.replaceOnPUT === true; by default sets to false, isn't it? Lets see:
If options.replaceOnPUT is undefined undefined === true => false
If options.replaceOnPUT is false false === true => false
If options.replaceOnPUT is true true === true => true

Am I missing something?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry, this is my fault, I overlooked === and thought it's =.

@Amir-61 Amir-61 assigned bajtos and 0candy and unassigned bajtos, Amir-61 and 0candy Jun 21, 2016
@bajtos

bajtos commented Jun 22, 2016

Copy link
Copy Markdown
Member

LGTM

@bajtos bajtos assigned Amir-61 and unassigned bajtos Jun 22, 2016
@Amir-61

Amir-61 commented Jun 23, 2016

Copy link
Copy Markdown
Member Author

@bajtos Lets please not merge this PR for now. I had a talk with @qpresley and we decided to have a discussion in F2F meeting about other IBM connectors (cloudant, informix. dashDB, DB2, DB2z, mqlight,...) support for replace methods before landing this PR. Mentioned connectors need to be tested as well; they are not on CI.

Thanks!

@Amir-61 Amir-61 changed the title Expose Replace* methods for 2.x DONOT MERGE-Expose Replace* methods for 2.x Jun 23, 2016
@Amir-61 Amir-61 changed the title DONOT MERGE-Expose Replace* methods for 2.x DO NOT MERGE-Expose Replace* methods for 2.x Jun 23, 2016
@Amir-61
Amir-61 force-pushed the expose_endpoints_2.x branch 2 times, most recently from 0dd7d54 to 1bf08db Compare August 10, 2016 02:00
@Amir-61

Amir-61 commented Aug 12, 2016

Copy link
Copy Markdown
Member Author

@laven26 @ssh24,

So I'm about to land this PR which exposes our replaceById, replaceAttributes and replaceOrCreate; if you are done with the QA for these methods for supported IBM DBs, could you please confirm it here, so I can land it this PR.

Thanks!

@ssh24

ssh24 commented Aug 12, 2016

Copy link
Copy Markdown
Contributor

mssql, mysql, oracl, mongodb and postgres all work as expected

@ghost

ghost commented Aug 12, 2016

Copy link
Copy Markdown

DB2, DashDB and Cloudant all work as expected

*Re-mapping `updateAttributes` endpoint to use
`PATCH` and `PUT`(configurable) verb
*Exposing `replaceById` and `replaceOrCreate` via
`POST` and `PUT`(configurable) verb
@Amir-61
Amir-61 force-pushed the expose_endpoints_2.x branch from 1bf08db to e562137 Compare August 15, 2016 16:17
@Amir-61 Amir-61 changed the title DO NOT MERGE-Expose Replace* methods for 2.x Expose Replace* methods for 2.x Aug 15, 2016
@Amir-61
Amir-61 merged commit 6c9df36 into 2.x Aug 15, 2016
@Amir-61
Amir-61 deleted the expose_endpoints_2.x branch August 15, 2016 19:05
@Amir-61 Amir-61 removed the #review label Aug 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants